Just noticed from #358 that there are existing plans to implement Taskwarrior-like complex filters. That might conflict with this PR.

Thanks @limdingwen for taking a stab at the problem!

Looking at your PR and reading your analysis (especially point 2, the boolean parser) it came to my mind that we are maybe doing the right things at the wrong place. Let me explain:

• We are using class Interval to convey filter information.
  An interval is an interval, and a filter is a filter. So they should be separate classes. For an interval it is fine that tags are a set and order does not matter. For a filter this may not be the case (I will come to this below)
• We are constructing the filter at the wrong place.
  IMHO the filter should be constructed futher up in the call chain where we have all information available. The filter should then be handed down to here.

Currently, this does not hurt much but it impedes improvements like the one you suggested.

Ideally, we should be doing something like this

for (; it != end; ++it)
{
    Interval interval = IntervalFactory::fromSerialization(*it);

    if (filter.accepts (interval))
    {
        intervals.push_back (std::move (interval));
    }
    else if (filter.done ())
    {
        break;
    }
}
return intervals;  

when we query intervals (see below, what the functions filter.accepts and filter.done do)

So I would propose the following path to achieve the goal of more flexible tag filtering:

1. Restructure the present functionality such it separates filter creation from filter application
2. Extend the filter creation such that it provides more flexible task filtering

For the first I would add an abstract base class which defines the functions outlined above

abstract class IntervalFilter
{
    abstract boolean accepts (Interval&) = 0;
    abstract boolean is_done() = 0;
}

From this I would derive two classes IntervalRangeFilter and IntervalTagSetFilter which implement the current behaviour.

• The IntervalRangeFilter is initialized with the given range, accepts all intervals within this range, and is done if the start date of the current interval is after its own range's end datetime.
• The IntervalTagSetFilter is initialized with the given tag set, accepts all intervals which contain at least one tag of its set, and is never done.

With that we can create the filter at the respective command and pass it to getTracked.

The next step would then be to create the real filter in the CLI where we have all the necessary information available and return it. This also means we have to change the current function CLI::getFilter to something like CLI::getRange/CLI::getTags/...

I think then we are in the best position to implement a more flexible tag filtering... 🤔

Keep in mind also, one of the nice features of timewarrior is that tags are more general strings, this is a thing taskwarrior can't really do. So if we start using - and OR, this can be a slippery slope. I'm using tags to store some properties of time intervals with the first character of the tag, which are typically punctuation chars, such as using + char to indicate a taskwarrior-mirrored tag, for example. (Maybe I could store this in the description though, which I currently don't use).

@smemsh Agreed, and thanks for the anecdote about "-tag" usage. While I'm not sure what would be a good way to do NOTs, I think ORs could be done as a sort of hint (:or).

@lauft Thanks for the warm welcome! Honestly, most of that stuff is going over my head right now since I'm new to the codebase, so I'll leave it for the others to discuss. If that refactoring is going to take a long time to make, I think it would help others to add the OR/NOT functionality first, without the boolean parser, since that seems to be most of what was requested for now in the referenced issues.

I might be biased, but long term I'd actually like the CLI syntax for timew and task grow closer, rather than grow apart. Taskwarrior already has support for things like "or" and "-" (exclusion), so it would make sense to adopt same syntactical elements in timew, and extend the tag specification (as @smemsh points out) in task. Then we could get these features basically for free, with a consistent UX for the users.

Copying over the relevant part of the docs from taskwarrior, since for others who may be unfamiliar with it too:

$ task project:Home and -work count
See the and operator between the filter terms? That is assumed to be there if not specfically stated. The unstated implication is that the disjunction ('or') is also supported.
$ task project:Home or -work count

@tbabej, sorry I'm new here so things may go over my head; are you proposing to move over the complex filter code from taskwarrior, when you said "get these features basically for free"?

Hello @limdingwen, a small update from my side:

I have pushed branch refactor/filtering with a first version of the new filtering. See CmdSummary for a first application. Converting the other commands should be easy.

... I think it would help others to add the OR/NOT functionality first, without the boolean parser ...

Well, your implementation already contains a boolean parser, though a simple one. 😉

The important thing is that we now can do the parsing (and filter construction) at the right place: close to the CLI. Whether we can integrate it completely into the CLI (and how) is up to the implementation.

I agree with @tbabej to align the CLI syntax, but we will have to address the issues addressed by @smemsh of using "or" as a tag, and having tags starting with - (or +, or...). (A similar issue to the former is the conflict of tags which are misinterpreted as dates, see #154, #60).

Maybe quoting could help here:

-"or" or "-FOO"

meaning matching all intervals not having tag or or having tag -FOO. 🤔

Full support with parantheses will most likely require adaptions to parsing itself as one cannot simply add a (/) into the command line – which in turn means we need to use quoting and then deconstruct the quoted tokens...

I agree with @tbabej to align the CLI syntax, but we will have to address the issues addressed by @smemsh of using "or" as a tag, and having tags starting with - (or +, or...)...

Just thinking out loud, but I wonder if #363 could help here. Then we add additional hints to give the parser clues.

For example, the following two lines could be equivalent:

# Produce a summary without intervals tagged "FOO"
timew summary -FOO
timew summary :not :tag FOO

Which would make things like the following work:

# Produce a summary without intervals tagged "-FOO"
timew summary :not :tag -FOO

@sruffell This would be a solution, but then we would definitely deviate from the current Taskwarrior CLI syntax. @tbabej what is your opinion about that?

I have updated branch refactor/filtering: Now getTracked(...) and getIntervalsById(...) use the new filtering – which also simplifies the DB interface a bit. Maybe also getLatestInterval(...) can also be replaced (by something like first_of (all_in_range ({ 0, 0 })))... 🤔

@limdingwen Do you think you can adapt your first version of the new tag filtering to this? Maybe at first only for CmdSummary, later for all others where applicable...

Thinking about the new tag filtering: a feature flag may be appropriate here, so we do not surprise any user.

@limdingwen I have done some thinking about the filtering, here are my findings so far:

There are three classes of filters:

• Id filters
• Tag filters
• Range filters

An Id filter excludes all other filters in the execution. If an id filter is used, only a range and/or a tag set are expected on the CLI. This does not mean a command cannot work with both, but it cannot use both at the same time (see continue).

I have compiled an overview what filters are used in the different commands in the following table (id means the command accepts only a single id, id+ meaning at least 2):

Most of the commands accept either and id filter or a tag and range filter - but there are some outliers:

• The continue command allows either a single id or tag filter, but no range filtering. The range is used to define the range of the interval to be created:

  $ timew continue @1  <from> - <to>  # create new interval <from> - <to> based on @1
  $ timew continue TAG <from> - <to>  # create new interval <from> - <to> based on first find with TAG
  

  Here, the error case of supplying an id and tag(s) on the CLI must be handled.

• The join command, in contrast to the others with id+ (which also accept a single id), only works with two ids.
  This means supplying only one id must result in an error message.

• The stop command uses the tag filter not to search for intervals but to filter the tag set of the current open interval to determine whether to create a new open interval:

  $ timew start FOO BAR BAZ
  $ timew stop  BAR BAZ     # creates closed interval 'FOO' 'BAR' 'BAZ', new open with 'FOO'
  

  So this might also be desired (but likely out of scope):

  $ timew start FOO BAR BAZ
  $ timew stop  -FOO        # creates closed interval 'FOO' 'BAR' 'BAZ', new open with 'FOO'
  

Hi @lauft, for now I've simply copy-pasted my basic OR/- code (without boolean parsing) into IntervalFilterAllWithTags. I'm glad to say it works rather well. This isn't intended to make it to release, but it shows the refactor is working with these changes. 😃

Based on my understanding, this current version would not support boolean parsing yet due to CLI::getFilter(Range) returning an Interval which does not contain ordered information, and more work needs to be done there. Is it intended such that CLI::getFilter(Range) will return a IntervalFilter instead to aid in this?

Also -- apologies, I may have borked this PR :( Still new to GitHub forking, deleted my fork to somehow get the new branch into my repo, and now GitHub doesn't seem to detect changes in my tag-operators branch. I'll have to close this PR and open a new one; would make sense too, since we're changing the base over to the refactoring branch instead.

New PR based on refactoring branch is here: #462